-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug fix for moving average tracking #48
Conversation
nathanielpritchard
commented
May 12, 2024
- Added more functionality to the moving average tracking to allow for use of a short width and long width moving average, which I thought worked but upon a closer look did not work.
res::Float64 = log.dist_info.scaling * (eltype(samp[1]) <: Int64 || size(samp[1],2) != 1 ? | ||
log.resid_norm(samp[3])^2 : log.resid_norm(dot(samp[1], x) - samp[2])^2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there so much white space added in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the time I thought it was more clear to have the if else statement within the parentheses aligned. I know see that is not the case and I have removed it.
# Check if MA is in lambda1 or lambda2 regime | ||
if ma_info.flag | ||
update_ma!(log, res, ma_info.lambda2, iter) | ||
else | ||
#Check if we can switch between lambda1 and lambda2 regime | ||
if res < ma_info.res_window[ma_info.idx] | ||
if iter == 0 || res <= ma_info.res_window[ma_info.idx] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line needs more explanation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have clarified that this lean is checking for a monotonic decrease in the sketched residuals
|
||
elseif ma_info.lambda == ma_info.lambda1 && !ma_info.flag | ||
diff = ma_info.idx - ma_info.lambda | ||
# Determine start point for first loop | ||
startp1 = diff < 0 ? 1 : (diff + 1) | ||
|
||
# Determine start and endpoints for second loop | ||
startp2 = diff < 0 ? ma_info.lambda2 + diff + 1 : 2 | ||
endp2 = diff < 0 ? ma_info.lambda2 : 1 | ||
# Compute the moving average two loop setup required when lambda < lambda2 | ||
for i in startp1:ma_info.idx | ||
accum += ma_info.res_window[i] | ||
accum2 += ma_info.res_window[i]^2 | ||
end | ||
|
||
for i in startp2:endp2 | ||
accum += ma_info.res_window[i] | ||
accum2 += ma_info.res_window[i]^2 | ||
end | ||
|
||
#Update the log variable with the information for this update | ||
if mod(iter, log.collection_rate) == 0 || iter == 0 | ||
push!(log.lambda_hist, ma_info.lambda) | ||
push!(log.resid_hist, accum / ma_info.lambda) | ||
push!(log.iota_hist, accum2 / ma_info.lambda) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conditional statement needs more commentary. When is this accessed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now that this is really unclear, I have added comments that I think clarify what is going on
estimate. By default, this is set to `1`. | ||
- `sigma2::Union{Float64, Nothing}`, the variance parameter in the sub-Exponential family. | ||
If not specified by the user, a value is selected from a table based on the `sampler`. | ||
If the `sampler` is not in the table, then ?. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace the ?
with default behavior
If the `sampler` is not in the table, then ?. | ||
- `omega::Union{Float64, Nothing}`, the exponential distrbution parameter. If not specified | ||
by the user, a value is selected from a table based on the `sampler`. | ||
If the `sampler` is not in the table, then ?. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace the ?
with default behavior